-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix shots argument of local_readout_mitigator #9155
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
48422a7
to
a1cb702
Compare
Pull Request Test Coverage Report for Build 3544051628
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert here, but I'm not sure this makes sense: in QuasiDistribution
the number of shots is provided as integer. Here it is a boolean? It seems like either the argument type here should also be an integer, otherwise it cannot be safely passed to the QuasiDistribution
.
The argument If the value of
I am not sure what the value of a non-default value for the |
The It's not necessary, hence it's type should be optional. |
I think that one should fix this: https://github.com/Qiskit/qiskit-terra/blob/4a1a2d0a79ba8cac77275326a32da117e9663c05/qiskit/result/mitigation/local_readout_mitigator.py#L172 to as in the base mitigator class: |
I'm not sure I understand what user-supplied shots actually mean in this context. The number of shots can be computed (as you do) from the counts data; passing other shots value not consistent with the count data cannot yield meaningful results, unless I'm missing something. Currently I think the parameter should simply be omitted. |
@gadial @ShellyGarion I will remove the Should we remove it altogether, or add a deprecation warning? |
If you remove However, I am not sure if the number of shots can always be calculated from the counts, as the |
Ok, in that case I will leave keep the argument (just update the typing from |
@peendebak - thanks for fixing this! |
…-terra into local_readout_mitigator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for fixing this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could we add a bugfix release note as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks!
* fix shots argument of local_readout_mitigator * fix correlated_readout_mitigator as well * add test * docstring * black * lint * add release notes * Fix up release note Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Summary
LocalReadoutMitigator.quasi_probabilities
method has an argument shots which is not used in the method. The docstring us updated for thisFixes #9154
Details and comments